Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: P4ADEV-1744-add-api-finalize-sync-status #15

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from

Conversation

LarissaASLeite
Copy link
Collaborator

Description

List of Changes

Motivation and Context

How Has This Been Tested?

  • Pre-Deploy Test
    • Unit
    • Integration (Narrow)
  • Post-Deploy Test
    • Isolated Microservice
    • Broader Integration
    • Acceptance
    • Performance & Load

Types of changes

  • PATCH - Bug fix (backwards compatible bug fixes)
  • MINOR - New feature (add functionality in a backwards compatible manner)
  • MAJOR - Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • CHORE - Minor Change (fix or feature that don't impact the functionality e.g. Documentation or lint configuration)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

…ync-status

# Conflicts:
#	openapi/generated.openapi.json
#	src/main/java/it/gov/pagopa/pu/debtpositions/mapper/DebtPositionMapper.java
#	src/main/java/it/gov/pagopa/pu/debtpositions/mapper/InstallmentMapper.java
#	src/main/java/it/gov/pagopa/pu/debtpositions/mapper/PaymentOptionMapper.java
#	src/main/java/it/gov/pagopa/pu/debtpositions/mapper/PersonMapper.java
#	src/main/java/it/gov/pagopa/pu/debtpositions/mapper/TransferMapper.java
#	src/main/java/it/gov/pagopa/pu/debtpositions/model/DebtPosition.java
#	src/main/java/it/gov/pagopa/pu/debtpositions/model/PaymentOption.java
#	src/main/java/it/gov/pagopa/pu/debtpositions/model/Transfer.java
#	src/main/java/it/gov/pagopa/pu/debtpositions/service/DebtPositionService.java
#	src/main/java/it/gov/pagopa/pu/debtpositions/service/DebtPositionServiceImpl.java
#	src/test/java/it/gov/pagopa/pu/debtpositions/util/faker/DebtPositionFaker.java
#	src/test/java/it/gov/pagopa/pu/debtpositions/util/faker/InstallmentFaker.java
#	src/test/java/it/gov/pagopa/pu/debtpositions/util/faker/PaymentOptionFaker.java
#	src/test/java/it/gov/pagopa/pu/debtpositions/util/faker/PersonFaker.java
#	src/test/java/it/gov/pagopa/pu/debtpositions/util/faker/TransferFaker.java
openapi/p4pa-debt-position.openapi.yaml Outdated Show resolved Hide resolved
Comment on lines 85 to 87
debtPosition.getPaymentOptions().forEach(this::updatePaymentOptionStatus);

updateDebtPositionStatus(debtPosition);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move these on a separate Service (DebtPositionHierarchyStatusAlignerService) which will set PaymentOptions and DebtPosition status based on their child.
The right logic is described on Confluence https://pagopa.atlassian.net/wiki/spaces/SPAC/pages/1420755133/Macchina+Stati+Posizione+Debitoria

PaymentOption status, prioritized order:

  • TO_SYNC -> if at least one Installment is TO_SYNC
  • PARTIALLY_PAID -> if at least one Installment is PAID and at least one is UNPAID or EXPIRED
  • UNPAID -> if all installments are UNPAID or CANCELLED
  • PAID -> if all installments are PAID or CANCELLED
  • REPORTED -> if all installments are REPORTED or CANCELLED
  • INVALID -> if all installments are INVALID or CANCELLED
  • CANCELLED -> if all installments are CANCELLED
  • EXPIRED -> if all installments are EXPIRED

DebtPosition status, prioritized order:

  • TO_SYNC -> if at least one PaymentOptions is TO_SYNC
  • PARTIALLY_PAID -> if at least one PaymentOptions is PAID and at least one is UNPAID or EXPIRED
  • UNPAID -> if all PaymentOptions are UNPAID or CANCELLED
  • PAID -> if all PaymentOptions are PAID or CANCELLED
  • REPORTED -> if all PaymentOptions are REPORTED or CANCELLED
  • INVALID -> if all PaymentOptions are INVALID or CANCELLED
  • CANCELLED -> if all PaymentOptions are CANCELLED
  • EXPIRED -> if all PaymentOptions are EXPIRED

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both update logic could be implemented on separate Services DebtPositionInnerStatusAlignerService and PaymentOptionInnerStatusAlignerService
group all these services on a common package

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's apply Chain Of Responsibility wiriting a separate class for each status, see https://github.com/pagopa/p4pa-payhub-activities/blob/main/src/main/java/it/gov/pagopa/payhub/activities/service/classifications/TransferClassificationService.java#L45 for an example of its application

(group these classes on nested packages, separated between DebtPosition and PaymentOptions statuses)

we could have the following structure:

  • service/statusalign/
    ** DebtPositionHierarchyStatusAlignerService
    ** DebtPositionInnerStatusAlignerService (no interface for this, because it should not be call outside)
    ** PaymentOptionInnerStatusAlignerService (no interface for this, because it should not be call outside)
    *** debtposition/
    **** DebtPositionStatusChecker (common interface implemented by all classes on this package, eg: DebtPositionToSyncStatusChecker)
    *** paymentoption/
    **** PaymentOptionStatusChecker (common interface implemented by all classes on this package)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each DebtPositionStatusChecker and PaymentOptionStatusChecker should have the @ordered annotation configured.

DebtPositionInnerStatusAlignerService and PaymentOptionInnerStatusAlignerService should accept the firstNull returned object

We should write 2 SpringBoot Test in order to check if the configuration of @order is rigth:

  • DebtPositionStatusCheckerTest
  • PaymentOptionStatusChecker
    These SpringBootTest will just autowire the List<.*StatusChecker> and the @test will verify the order
    (Possibly, let's configure the test to scan just the service/statusalign/ package)

Comment on lines 85 to 87
debtPosition.getPaymentOptions().forEach(this::updatePaymentOptionStatus);

updateDebtPositionStatus(debtPosition);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each DebtPositionStatusChecker and PaymentOptionStatusChecker should have the @ordered annotation configured.

DebtPositionInnerStatusAlignerService and PaymentOptionInnerStatusAlignerService should accept the firstNull returned object

We should write 2 SpringBoot Test in order to check if the configuration of @order is rigth:

  • DebtPositionStatusCheckerTest
  • PaymentOptionStatusChecker
    These SpringBootTest will just autowire the List<.*StatusChecker> and the @test will verify the order
    (Possibly, let's configure the test to scan just the service/statusalign/ package)

- INVALID
- EXPIRED
- UNPAID
SyncStatusDTO:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
SyncStatusDTO:
SyncStatusUpdateRequestDTO:

content:
application/json:
schema:
$ref: "#/components/schemas/SyncStatusDTO"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$ref: "#/components/schemas/SyncStatusDTO"
$ref: "#/components/schemas/SyncStatusUpdateRequestDTO"

Comment on lines 42 to 43
.filter(installment -> TO_SYNC.equals(installment.getStatus()))
.filter(installment -> syncStatusDTO.containsKey(installment.getIud()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you put just 1 filter operation in order to:

  • log as ERROR a TO_SYNC installment not present in the map
  • log as ERROR an installment in the map, but without TO_SYNC status
    The filter will return the AND between the actually configured conditions

IudSyncStatusUpdateDTO updateDTO = syncStatusDTO.get(installment.getIud());

try {
InstallmentStatus newStatus = InstallmentStatus.valueOf(updateDTO.getNewStatus().toUpperCase());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this conversion still necessary? aren't they the same enum type?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, in the map it's still a string, but i can change it

Comment on lines 55 to 57
} catch (IllegalArgumentException e) {
throw new InvalidStatusException("Invalid status: " + updateDTO.getNewStatus());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if same enum, this catch should not be necessary

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the enums are the same, this exception could not more be used

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i still use it in the class PaymentOptionStatusChecker at the else condition:

 else {
      throw new InvalidStatusException("Unable to determine status for PaymentOption");
    }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if InvalidStatusException could be removed, let's not delete this, but let's handle some other exception here, (eg MethodArgumentNotValidException)

repositoryUpdater.accept(entity, newStatus);
}

public boolean isToSync(List<E> statusList) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you rename all the parameters statusList into childrenStatuses or childrenStatusList

Comment on lines 50 to 52
return allMatchOrEmpty(statusList, status -> status.equals(unpaidStatus)) ||
(allMatchOrEmpty(statusList, status -> status.equals(cancelledStatus) || status.equals(unpaidStatus)) &&
statusList.contains(unpaidStatus));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not efficient, you are iterating statusList for each status!

what you should implement is:
statusList.contains(unpaidStatus) && statusList.stream().allMatch(status -> status.equals(cancelledStatus) || status.equals(unpaidStatus));

you could generalize it the following way:

private allMatch(List<E> statusList, E requiredState, Set<E> allowedStatuses){
statusList.contains(requiredState) && statusList.stream().allMatch(status -> requiredState.equals(status) || allowedStatuses.contains(status));
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants